-
Notifications
You must be signed in to change notification settings - Fork 46.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for the issue #5189 #5209
Fix for the issue #5189 #5209
Conversation
@@ -213,6 +221,10 @@ var ReactDefaultPerf = { | |||
totalTime, | |||
Array.prototype.slice.call(args, 1) | |||
); | |||
|
|||
if (fnName === 'putListener') { | |||
ReactDefaultPerf._prevEventListener = typeof isFunction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skeptical this would work. I think there is a single ReactDefaultPerf shared for all components; you are setting this field as a static field without regard to the current component.
@antoaravinth Looks like there is a failing unit test. Also, we should add a unit test that asserts the intended semantics. @spicyj is probably the best person to review this PR, since he added the processing of event handlers to printDOM() in the first place. |
I would just not instrument putListener/deleteListener at all. |
@jimfb: The moment we are going to ignore the "putListener" events, the test case @spicyj Will do the same. |
Yes, we can remove that test. |
@antoaravinth updated the pull request. |
@antoaravinth updated the pull request. |
Just remove the instrumentation from EventPluginHub entirely – we don't need it any more. |
Sorry for the back and forth. |
Can you reference #5189 in your new test case too? It would also be fine to combine them into one test, I think – but two is fine too if you prefer that. |
@spicyj No need to be sorry, thanks for your help on the PR. Just removing the instrumentation lines :
from But at the same time what about the line
I guess we can even safely remove that. Because we removed our instrumentation, the above condition is useless there. Throw your thoughts on this. |
@antoaravinth Yes, that's right – all that code can go away. |
Fix for lint issues Added the test cases for the issue 5189 Removing empty space Removing the Eventplugin instrumentation code Removing unwanted white space
@antoaravinth updated the pull request. |
Don't instrument listeners for ReactDefaultPerf
Thanks! |
@jimfb I have written logic for the issue mentioned over here:
#5189
Let me know my thought process is correct; If so I can go and write unit test case for the same.
Thanks for your help.